fix(i18n): require an explicit locale on presentation helpers - #4640
Conversation
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
The call-site claim holds up: ci.yml runs npm run typecheck across the workspaces whenever code changes, packages/ui/stories is covered through apps/desktop/tsconfig.storybook.json, and nothing outside the typechecked configs calls these helpers (no .mjs, .js or e2e caller), so a green CI does prove every caller was updated. Both packages are private, so removing the defaults breaks no published surface. Two things to settle first.
P2: the sweep only covered 'zh', so the title's claim does not hold. The verification grep was "locale: UiLocale = 'zh'|locale ?? 'zh'", which misses the same defect spelled with double quotes and the other locale. Still on head: packages/ui/src/materialize.ts:153 (materializeChat(messages, locale: UiLocale = "en")), packages/ui/src/materialize.ts:667 (materializeTurns), and apps/desktop/src/renderer/app-shell-context-compaction.ts:35 (contextCompactionNotice(outcome, uiLocale: UiLocale = 'en')). The two materialize helpers are the main transcript presentation path, and they are not in #4524's file list (that PR touches materialize.test.ts but not materialize.ts) or #4551's, so no follow-up currently claims them. A caller that forgets renders English to Chinese users: the direction is flipped, the defect is the same. Either fold them in or name them in the body as deliberately out of scope, otherwise this reads as complete and the #2672 item gets closed while three helpers still default.
P3: nothing stops the next one. This PR and #4524 and #4551 all rely on a one-off grep; there is no lint rule or check-renderer-architecture entry asserting that a UiLocale parameter carries no default. Adding one is what actually closes this class out. Not blocking.
P3: two claims in the description do not hold, and I would rather they were stated accurately than dropped. "Tests cover the change and fail without it" is not the case: restore the defaults and all four touched test files still pass, because passing 'zh' explicitly gives the same result and the new 'en' assertions pass either way. The guard for this change is tsc, which is a fine answer, just a different one. And "each touched helper gained one 'en' assertion" is 4 assertions against 24 helpers; even among the tested ones, describeTurnErrorClass (session-error-presentation.test.ts:31) still only has a 'zh' assertion.
No conflict or ordering dependency with #4641: git merge-tree on the two heads is clean, and the only shared file is provider-panel-shared.ts, where #4640 changes signatures and #4641 deletes a branch inside one body. Worth knowing that #4551 also edits that file plus settings-provider-copy.ts, so whichever of the three lands last rebases once.
0877e02 to
d98ab14
Compare
A locale default let any caller that forgot the argument render the wrong language without a type error: 'zh' defaults showed Chinese to English users, 'en' defaults the reverse. Generated-by: Claude Code
d98ab14 to
7b5bdd8
Compare
|
@Astro-Han Comments are addressed.
|
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed at 7b5bdd8. Everything from the last round is closed: materializeChat / materializeTurns / contextCompactionNotice are required now, the unreachable now / depth defaults are gone, and the body says plainly that tsc is the guard. I re-checked the production callers that used to lean on a default (chat-view.tsx, app-shell-turn-view-model.ts, quote-companion-panel.tsx, artifact-pane.tsx, tasks-settings-page.tsx, session-history-list.tsx) and every one passes the locale it has in scope, so runtime behavior is unchanged. The ledger delta is the two = 'en' tokens, downward only.
One number: the body says 27 helpers, the diff removes 33 defaults plus makes three locale?: fields required. Worth fixing before squash since the title becomes the commit.
Ordering: #4823 edits the same app-shell-context-compaction.ts entry in renderer-architecture.json two lines away from your change, so whichever lands second should regenerate the ledger rather than resolve the conflict by hand. No dependency either way.
AI-assisted review: drafted with Maka; I verified the caller list and the ledger delta myself.
2 participants
Summary
Presentation helpers across desktop renderer,
@maka/core, and@maka/uidefaulted theirlocaleparameter, so a caller that forgot to pass it rendered the wrong language with no type error:'zh'defaults showed Chinese to English users, and the"en"defaults onmaterializeChat/materializeTurns/contextCompactionNoticedid the reverse. This PR removes every such default (27 helpers) so the locale is a required argument, and makesTranscriptProjectionInput.localerequired for the same reason. No production caller relied on a default — every one already passed the locale it had in scope — so the runtime behavior is unchanged; only the type-level guarantee is new. Tests that leaned on a default now pass the locale explicitly.Where a required
localenow follows an optional parameter (nowin the three relative-time helpers,depthinformatAsKeyValueLines), that leading default is dropped too: it became unreachable and every caller already passed it.The guard for this change is
tsc, not the tests: restoring a default keeps the existing assertions green. The'en'assertions added to the touched test files prove the English path for those helpers only; they are not a per-helper sweep.The six
@maka/uistreaming helpers with the same defect are fixed in #4524 and are not touched here; the two OAuth result helpers are fixed in #4551. A lint rule that rejects any futureUiLocaledefault is left for a follow-up.Refs #2672
Verification
AI use
Select exactly one:
Tool(s) and scope: Claude Code — implementation, tests, and this description, under the contributor's direction; the commit carries a
Generated-by: Claude Codetrailer.Checklist
tsc; see Summary)